Skip to content

refactor(table): split the table hierarchy and materialize the store a table serves - #447

Open
lucasfang wants to merge 27 commits into
apache:mainfrom
lucasfang:dev30
Open

lucasfang wants to merge 27 commits into
apache:mainfrom
lucasfang:dev30

Conversation

@lucasfang

@lucasfang lucasfang commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

Purpose

Linked issue: close #459

The single Table class dispatched managed, format, and system tables through if branches in one 1400-line table.cpp; every operator call re-merged an options map and re-parsed CoreOptions (Java's AbstractFileStore parses once), and the served file system was resolved before the schema's access configuration was known. This splits the hierarchy the way Java splits it and gives context options one clear place to speak, fixing the defects that the ambiguity hid.

Changes:

  • Table (include/paimon/catalog/table.h) becomes an abstract base: Name/FullName/NewXxx/Copy are virtual, the constructor is protected. Implemented by FileStoreTable (with AppendOnlyFileStoreTable/PrimaryKeyFileStoreTable subclasses, src/paimon/core/table/), by FormatTable (now deriving from Table, owning its own assembly — format_table_loader is absorbed), and by the SystemTable family.
  • New FileStore (src/paimon/core/table/file_store.h/.cpp), the counterpart of Java's AbstractFileStore: materializes CoreOptions once through a static Create(), resolves the served file system after the options are known (schema-carried access configuration names its own backend; the caller's explicit file system wins over it), carries the file-system scheme map, and hosts the operator factories. FileStoreTable holds its schema/snapshot managers and its branch; Copy() hands the managers over unless a file-system option among the copy's changes the resolution, and never half-applies type or branch; SwitchToBranch() derives a table of another branch.
  • Context options speak at the load: a factory-path load (TableScan::Create(ctx) etc.) merges them over the schema's through Table::Copy() — a table is the product of its load — while NewXxx on a held table refuses a contradicting option (option 'X' is 'A' in the context but 'B' on the table, values redacted through SensitiveConfigUtils::RedactValue). The four context builders document the semantics.
  • RestCatalog (src/paimon/rest/rest_catalog.cpp): GetTable serves the global system tables of the sys database locally (they previously reached the managed-table load, which statically cast their read-only schema — undefined behavior), binds per-table system tables to GetTableFileSystem() so data-token credentials reach $snapshots and friends, and FileStoreTable::Create refuses a non-managed schema instead of casting it.
  • System table views (read_optimized_system_table.cpp, audit_log_system_table.cpp): the underlying scans and reads prefer the context's file system and fall back to the table's own only when the context names neither a file system nor a scheme map (the previous unconditional fallback was refused when a scheme map was present). The scan-fallback read context merges the load's dynamic options, so the fallback reader recognises a sequence-number column the main one does.

Tests

  • src/paimon/core/table/table_test.cpp (expanded): managers of the branch a table was loaded with, SwitchToBranch, Copy keeping that branch when handed a branch option (both directions, verified to fail before the fix), operators refusing an option that contradicts a held table, a factory-path override of a schema-carried key, cache injection, the sys-database REST open, and the per-table file system.
  • src/paimon/core/table/source/table_read_test.cpp (TestMergeOptions, TestDependencyReadWithoutCatalogUsesReadOptions): factory-path merging of context options into the served schema, and the dependency catalog context seeing the merged header value.
  • src/paimon/core/catalog/file_system_catalog_test.cpp, src/paimon/rest/rest_catalog_test.cpp, and the inte tests (scan_and_read, realtime_write, read, scan, write_and_read_inte_with_index) adapted to the Table API.
  • Ran (with PAIMON_ENABLE_REST=ON): paimon-core-test 2492/2492; paimon-rest-test 152/152; ctest -j 96 32/33 — the only failure is paimon-write-and-read-inte-test with its 2 ("parquet", "jindo") jindo-credential environment cases, unchanged from the baseline; paimon-data-evolution-table-test 306/306 and paimon-scan-and-read-inte-test 511/511; pre-commit run --files <changed> passed (clang-format applied); git diff --check clean.

API and Format

No storage format or protocol change. Breaking public API changes: Table is now abstract (constructor protected, Name/FullName/NewXxx/Copy virtual, Table::CurrentBranch() replaced by FileStoreTable::CurrentBranch()); FormatTable derives from Table (static FormatTable::Copy(table, options) becomes the virtual Table::Copy(options), Location() becomes GetRootPath()); FileStoreTable::SwitchToBranch is new. Observable semantics: schema options that do not parse fail at table-build time (previously at the first operator); a table without a schema-carried backend or a caller file system is served by the default local one instead of a null one; context options are merged at load time only.

Documentation

No new user-facing feature; the option semantics on the four context builders are documented in the same change, and docs/source/user_guide/format_table.rst follows the FormatTable API rename.

Generative AI tooling

Generated-by: Qoder

@lucasfang lucasfang changed the title refactor table refactor: refactor table abstraction Oct 20, 2026
@lucasfang
lucasfang marked this pull request as draft October 20, 2026 07:00
@lucasfang
lucasfang force-pushed the dev30 branch 2 times, most recently from f93aa25 to 7b35032 Compare October 23, 2026 07:06
@lucasfang lucasfang changed the title refactor: refactor table abstraction refactor(table): split the table hierarchy and materialize the store a table serves Oct 23, 2026
@lucasfang
lucasfang marked this pull request as ready for review October 23, 2026 09:32

@SteNicholas SteNicholas left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucasfang, I left some comments for this pull request. PTAL.

Comment thread src/paimon/rest/rest_catalog.cpp Resolved
Comment thread src/paimon/core/table/file_store.h Resolved
Comment thread docs/source/user_guide/format_table.rst Resolved
Comment thread include/paimon/read_context.h Resolved
Comment thread src/paimon/core/table/file_store.h Resolved
Comment thread src/paimon/rest/rest_catalog.cpp Resolved

@SteNicholas SteNicholas left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucasfang , one additional correctness issue remains in the direct system-table read path.

Comment thread include/paimon/catalog/table.h Resolved
Comment thread src/paimon/core/table/file_store.cpp Resolved

@zjw1111 zjw1111 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two correctness issues remain in the table-first write/commit path at 13fba3780476ce4177d2197e0d7f62816bdd3047.

@zjw1111 zjw1111 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One branch identity issue remains in SwitchToBranch() at 13fba3780476ce4177d2197e0d7f62816bdd3047.

@SteNicholas SteNicholas left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucasfang, thanks for updates. Only one fallback-read correctness issue comment. PTAL.

@SteNicholas SteNicholas left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucasfang, thanks for updates. I left three comments for correctness issues remaining at ba063c6: explicit context branch validation, dynamic options for system-table views, and accumulated overrides when copying to another branch.

Comment on lines +366 to +368
PAIMON_RETURN_NOT_OK(CheckContextNamesThisTable("write", ctx->GetRootPath(),
ctx->GetIdentifier(),
/*context_table_id=*/std::nullopt));

@SteNicholas SteNicholas Oct 29, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reject explicit context branches that differ from the table. A main-branch table accepts a WriteContext built with WithBranch("dev") because this validation checks only the path and identifier. NewCommit() similarly accepts AddOption(Options::BRANCH, "dev") when the schema has no branch option. Both operators then use the store's main-branch managers, so a write intended for dev can be published to main without an error. Please validate explicitly supplied branch settings against CurrentBranch() before constructing either operator, and cover this case through the table-based write and commit entry points.

Comment on lines +216 to +218
Result<std::shared_ptr<Table>> SystemTable::Copy(
const std::map<std::string, std::string>& /*dynamic_options*/) const {
return Status::NotImplemented(Name(), " system table is read-only: copying is not supported");

@SteNicholas SteNicholas Oct 29, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preserve dynamic scan options for system-table views. Path-based factories now reject system tables, but this unconditional rejection also prevents configuring their replacement catalog-loaded handles. For example, NewScan() on tbl$ro ignores context options scan.mode=from-snapshot and scan.snapshot-id, using only the options captured when the table was loaded; Copy() cannot apply them either. A historical scan consequently reads the latest snapshot. Please preserve a supported way to apply dynamic options to read_optimized and audit_log views, and test the selected snapshot through Catalog::GetTable().

Comment on lines +200 to +207
PAIMON_RETURN_NOT_OK(SchemaValidation::CheckImmutableOptions(*base_schema, overridable));
PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<TableSchema> merged_schema,
base_schema->MergeOptions(schema_overridable));
std::map<std::string, std::string> accumulated = dynamic_options_;
accumulated.erase(Options::TYPE);
accumulated.erase(Options::BRANCH);
for (const auto& [key, value] : overridable) {
accumulated.insert_or_assign(key, value);

@SteNicholas SteNicholas Oct 29, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge accumulated overrides into a branch copy's schema. When Copy() switches branches, base_schema becomes the target branch's persisted schema, but MergeOptions() receives only this call's overrides. Earlier dynamic_options_ are retained in accumulated without reaching the new store's CoreOptions. For example, Copy({{Options::KEY_VALUE_SEQUENCE_NUMBER_ENABLED, "true"}}) followed by Copy({{Options::BRANCH, "dev"}}) loses sequence-number support when dev omits that setting, causing an _SEQUENCE_NUMBER projection to fail even though option validation still considers the setting enabled. Please merge the accumulated overrides consistently into the target schema and add a regression test for successive copies.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Split the table hierarchy and materialize the store a table is served by

5 participants